Type size v2 #62
Type size v2 #62
Conversation
…ing on the conversion
…he opposite name)
src/module.rs
Outdated
| dl.get_type_alloc_size_in_bits( | ||
| &types, | ||
| &Type::StructType { | ||
| element_types: vec![types.i8(), types.pointer_to(types.i8())], |
There was a problem hiding this comment.
With LLVM 15, typed pointers were removed in favour of untyped pointers. Since the type doesn't matter here, you could have a helper function along the lines of:
fn get_pointer_type(types : &Types) -> TypeRef {
#[cfg(feature = "llvm-14-or-lower")]
{
types.pointer_to(types.i8())
}
#[cfg(feature = "llvm-15-or-greater")]
{
types.pointer()
}
}or something like that, to get it to work with both typed and typeless pointers
|
Not the maintainer, but wanted to note a few things:
|
Signed-off-by: Joel Imbergamo <joel.imbergamo@atos.net>
Signed-off-by: Joel Imbergamo <joel.imbergamo@atos.net>
…d Type::VectorType scalable field Signed-off-by: Joel Imbergamo <joel.imbergamo@atos.net>
…s where it was false Signed-off-by: Joel Imbergamo <joel.imbergamo@atos.net>
Signed-off-by: Joel Imbergamo <joel.imbergamo@atos.net>
|
I've taken into account your comments, and added some tests, the CI is not working on the repo bc a maintainer needs to allow it but it runs on my repo for all llvm versions now. Let me know if you have any other remarks, I am happy to apply them! If not we'll wait for the owner to respond and hopefully merge it if everything is ok! (I might need to clean the git history a bit I've made a mess). |
| // --TODO not yet implemented-- pub metadata_nodes: V | ||
| // Type::MetadataType => None, | ||
| // Type::TokenType => None, | ||
| // Type::VoidType => None, | ||
| // Type::TargetExtType => todo!(),ec<(MetadataNodeID, MetadataNode)>, |
|
|
||
|
|
| #[cfg(feature = "llvm-16-or-greater")] | ||
| Type::TargetExtType => todo!(), |
There was a problem hiding this comment.
What does the LLVM method for get-alignment-of-type do for target ext types? Neither the LangRef for target ext types nor the LangRef for data layout seem to specify the alignment of these types.
| #[derive(Copy, Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] | ||
| pub struct TypeSize { |
There was a problem hiding this comment.
Can we get a doc comment on this struct?
|
|
||
| #[derive(Copy, Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] | ||
| pub struct TypeSize { | ||
| quantity: u64, |
There was a problem hiding this comment.
Instead of quantity, can we have bits or quantity_bits to make clear this is bits not bytes? (Compare to get_type_store_size() above, where not specifying defaults to bytes, but here, not specifying seems to default to bits.)
| impl From<&Types> for TypeRef { | ||
| fn from(types: &Types) -> Self { | ||
| #[cfg(feature = "llvm-14-or-lower")] | ||
| { | ||
| types.pointer_to(types.i8()) | ||
| } | ||
| #[cfg(feature = "llvm-15-or-greater")] | ||
| { | ||
| types.pointer() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think this should be a From; it's not clear why types.into() should produce a pointer type and not some other kind of type. Is it sufficient to use the existing method Types::pointer_to(types.i8())?
This pull requests continues the work of @langston-barrett in #34 . I have taken into account the concerns you expressed in that pull request and rebased it to the current main branch.
I was not able to to address this concern:
Can we have a comment pointing to the upstream function this is a port of, similar to how you did for getTypeSizeInBits? as I am not sure if this function is equivalent to the llvm function I have been able to identify (https://llvm.org/doxygen/DataLayout_8cpp_source.html#l00553).
And I would like feedback on how to deal with Type::TargetExtType as we don't have access to the type's data as explained in the enum.
I am no expert on llvm so any feedback is appreciated.
As a last note, I've seen the confusion between bits and bites multiple times in the code (I've hopefully fixed them), but upon successful validation of this code, a bit and byte types should be created moving forward.